Skip to content
This repository has been archived by the owner on Jun 11, 2020. It is now read-only.

Add gitHead to package.json #543

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

felixfbecker
Copy link

@felixfbecker felixfbecker commented Dec 7, 2018

This is a field normally added automatically by npm publish. It is extremely useful to map packages back to a revision in the git repository (especially when releases are not tagged). Sourcegraph uses it to provide cross-repository go-to-definition.

Closes #458

This is a field normally added automatically by npm publish.

Closes microsoft#458
@felixfbecker
Copy link
Author

@sandersn before I solve the merge conflict, could you maybe take a look at this PR?

@sandersn sandersn self-requested a review December 20, 2018 23:09
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is a good one, but I think the SHA should be specific to the project that has changed.

@@ -52,11 +52,12 @@ async function outputFilePath(pkg: AnyPackage, filename: string): Promise<string

interface Dependencies { [name: string]: string; }

async function createPackageJSON(typing: TypingsData, version: string, packages: AllPackages): Promise<string> {
async function createPackageJSON(typing: TypingsData, version: string, gitHead: string, packages: AllPackages): Promise<string> {
// Use the ordering of fields from https://docs.npmjs.com/files/package.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the documentation doesn't mention gitHead, so I suspect this comment isn't true for gitHead. I'm not sure whether that matters though.

Any idea how to find out where npm's package.json puts gitHead? I tried looking at unpgk's version but it doesn't appear to be there either.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can find it in the registry API response: http://registry.npmjs.org/commander/latest
And npm should also write it out in node_modules when installing.
npm info commander gitHead also returns it (or npm info commander --json).

But the order of keys shouldn't really matter in an object.

It gets added by npm here: https://github.com/npm/read-package-json/blob/1b85cf87f5875774e7fbc617f606923d8cbda6a4/read-json.js#L345

* Return the commit SHA1 of the master tip of DefinitelyTyped
*/
export async function resolveDefinitelyTypedMaster(githubAccessToken: string, fetcher: Fetcher): Promise<string> {
const masterRef = await queryGithub("repos/DefinitelyTyped/DefinitelyTyped/git/refs/heads/master", githubAccessToken, fetcher) as { object: { sha: string } };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is master the correct SHA to use here? In particular, it's possible that it may have moved between the time that this package was merged. Another package may have been merged in between.

I think it would be better if you use the latest sha just for the package's directory.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really matter - this SHA will be used to download the zip, so it is guaranteed that gitHead points to the exact revision that is being published. That is all that should matter.

It would be nice, but I wouldn't know where to get that commit from. Can you point to where?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s the code I used, although that was using octokit. The rest call shouldn’t be too different though.

DefinitelyTyped/types-publisher-watchdog@f647115#diff-168726dbe96b3ce427e7fedce31bb0bcL76

Look at the part I deleted starting with const oops

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so you would get the latest commit from the git log of that folder. I thought you were suggesting to use the commit ID from the triggering event, e.g. the commit hash of the merge commit of a PR that was merged which triggered the new publish

@sandersn
Copy link
Member

I also thought it would be easier for me to understand the change with a test — I added jest a little while ago. If you don’t have time for that I can try to add it on Monday.

@sandersn
Copy link
Member

Here's the test I added. However, I can't get github API requests to work at all (eg from npm run get-definitely-typed), so I haven't been able to verify that it passes.

+    async definitelyTypedMasterResolvesToCommit() {
+        expect(process.env["GH_API_TOKEN"]).toBeDefined();
+        const s = await resolveDefinitelyTypedMaster(process.env["GH_API_TOKEN"]!, new Fetcher());
+        expect(typeof s).toBe("string");
+        expect(s.length).toBe(40);
+        expect(/[^a-z0-9]/.test(s)).toBe(true);
+    }

I'll push a commit myself if I can get github API requests working but I don't have any more time today to do so.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants